Conversion of Service Dialogs Form from Angular to React #9592

elsamaryv · 2025-08-25T10:48:19Z

Revamp of service dialogs (Also includes its conversion from Angular to React)

@miq-bot add-label enhancement
@miq-bot add-label wip

Fryguy · 2025-08-25T15:34:50Z

cypress/support/commands/common.js

@@ -0,0 +1,37 @@
+/* eslint-disable no-undef */
+
+// Look for notification popups and disable them if present


Are these for "real" notifications or for debug-type notifications? If the latter, then this shouldn't be needed because if you run the webpack-dev-server and the rails server with CYPRESS=true then those debug-type notifications won't be there.

Fryguy · 2025-08-25T15:36:02Z

cypress/support/commands/customization/service-dialogs.js

+/* eslint-disable no-undef */
+
+// Drag and drop a component to a section
+Cypress.Commands.add('dragAndDropComponent', (componentName, targetSectionIndex = 0) => {


Because there are commands being defined globally, but are specific to service dialogs, I think the names should be prefixed with something like serviceDialog to avoid any name collisions and to clarify their purpose.

Fryguy · 2025-08-25T15:36:33Z

cypress/support/commands/customization/service-dialogs.js

+});
+
+// Login and navigate to add a new service dialog
+Cypress.Commands.add('navigateToAddDialog', () => {


well, maybe not this one because it also includes login and navigation

app/javascript/packs/component-definitions-common.js

Fryguy · 2025-08-25T15:38:56Z

I know this is still WIP, but can @GilbertCherrie , @asirvadAbrahamVarghese please review to get a first pass on it?

app/javascript/components/common/inline-flash-message/index.jsx

asirvadAbrahamVarghese · 2025-08-27T03:49:46Z

app/javascript/components/common/inline-flash-message/index.jsx

+import PropTypes from 'prop-types';
+import { InlineNotification } from 'carbon-components-react';
+
+const InlineFlashMessage = ({ message, setMessage }) => {


if it’s just to pass through any actions required for the close button click maybe we could rename the prop to something like onCloseClick (I’m clueless when it comes to naming, so it’s your call)

Should we consider changing message to messageInfo, given that its an object? - again thats your call

if it’s just to pass through any actions required for the close button click maybe we could rename the prop to something like onCloseClick

This component pulls InlineNotification component from carbon-components-react and is used to display contextual messages inline with other contents in the UI. onCloseButtonClick is actually a property of the InlineNotification component - It is an event handler that is only called when the user clicks the close button(if it is visible).

I meant the setMessage prop from our component that we’re using as the handler for onCloseButtonClick in InlineNotification, I feel like onCloseButtonClick ={onCloseClick} (or anything similar) seems better in this context

asirvadAbrahamVarghese · 2025-08-27T03:50:03Z

app/javascript/components/common/inline-flash-message/index.jsx

+      title={message.title || ''}
+      subtitle={message.subtitle || ''}
+      lowContrast
+      hideCloseButton={!setMessage}


It might be better to control hideCloseButton through a separate boolean prop and set a default for it 🤔

asirvadAbrahamVarghese · 2025-08-27T03:50:12Z

app/javascript/components/common/inline-flash-message/index.jsx

+      lowContrast
+      hideCloseButton={!setMessage}
+      onCloseButtonClick={
+        typeof setMessage === 'function' ? () => setMessage(null) : undefined


I was thinking if we should directly do:
onCloseButtonClick={setMessage}
So that react will pass down the event automatically when setMessage handler is called similar to how we’d pass the event manually like:
onCloseButtonClick={(event)=>setMessage(event)}

So if in case we want to access any event methods (preventDefault, stopPropagation) from this component sometime later we can do:
<InlineFlashMessage setMessage={(event)=>{ event.preventDefault(); }} .... />

asirvadAbrahamVarghese · 2025-08-27T03:50:43Z

app/javascript/components/common/inline-flash-message/index.jsx

+};
+
+InlineFlashMessage.defaultProps = {
+  message: null,


since we are doing if (!message) return null; at the top of the component, we can safely skip setting message as null from default props unless its marked required by PropTypes.shape({}).isRequired - the value received will be undefined and would still match our if(!message) condition

asirvadAbrahamVarghese · 2025-08-27T03:51:52Z

app/javascript/components/common/inline-flash-message/index.jsx

+
+InlineFlashMessage.defaultProps = {
+  message: null,
+  setMessage: undefined,


We can set a no-op function as default instead of undefined:
setMessage: () => {},
If thats throwing any eslint error(empty blocks), we can do:

setMessage: () => { // default callback },

so that it prevents errors like “Cannot call setMessage of undefined”.
this also means we don’t have to explicitly verify typeof setMessage === 'function' before passing existing function reference directly: onCloseButtonClick={setMessage}
or even if we are creating a new arrow function on every render that calls onCloseButtonClick:

onCloseButtonClick={() => { // any other action setMessage(); }}

asirvadAbrahamVarghese · 2025-09-01T06:17:55Z

app/javascript/components/date-time-picker/index.jsx

+} from 'carbon-components-react';
+import { getCurrentDate, getCurrentTimeAndPeriod } from '../service-dialog-form/helper';
+
+const CustomDateTimePicker = (field) => {


Correct me here if I am wrong
We will be using the component like:

<CustomDateTimePicker label="Custom Date Time Picker" initialData={{ date: '11/06/2025', time: '23:11', period: 'PM' }} onChange={(data) => { // any actions with on change data }} />

So I was thinking - just like we did with InlineFlashMessage, inline destructuring of component props is generally cleaner instead of accessing them via field object:
const CustomDateTimePicker = ({ label, onChange, initialData }) => {...

and then add props validations and default props for them:

CustomDateTimePicker.propTypes = { initialData: PropTypes.shape({ date: PropTypes.string, time: PropTypes.string, period: PropTypes.string, }), onChange: PropTypes.func, label: PropTypes.string, }; CustomDateTimePicker.defaultProps = { initialData: { date: '', time: '', period: '' }, onChange: () => {}, label: '', };

Note: date, time and period having an empty string as default value ({ date: '', time: '', period: '' }) shouldn't be a problem as they are falsy in JS, fallback methods like getCurrentDate and getCurrentTimeAndPeriod will still be triggered to set states initial values

asirvadAbrahamVarghese · 2025-09-01T06:18:14Z

app/javascript/components/date-time-picker/index.jsx

+const CustomDateTimePicker = (field) => {
+  const { initialData, onChange } = field;
+
+  const [date, setDate] = useState(initialData.date || getCurrentDate);


I guess it would be better to call the function immediately (getCurrentDate) instead of passing the reference for React to call it automatically to get the initial state value when initialData.date is not available:
const [date, setDate] = useState(() => initialData.date || getCurrentDate());

also include the lazy initialization () =>🔝, like we did with time and period states initializations, so that the arrow function is only executed during the initial render, on subsequent re-renders, react knows it already has a state value and doesn't need to call the function again. The performance difference would be negligible here I believe, but it's better to use the arrow function for any initialization that creates objects, makes calculations or anything that could potentially be expensive.

asirvadAbrahamVarghese · 2025-09-01T06:19:18Z

app/javascript/components/date-time-picker/index.jsx

+  const { initialData, onChange } = field;
+
+  const [date, setDate] = useState(initialData.date || getCurrentDate);
+  const [time, setTime] = useState(() => initialData.time || getCurrentTimeAndPeriod().time);


On initial render, time would look like this which is not in our expected "hh:mm" format, it should have been "09:54", But the invalid time error: "Enter a valid 12-hour time" is not displayed, probably thats because isValid state is initially true

Right after time field is edited the error is displayed since our state isValid state is now updated:

No issues with this implementation, this should work well once we apply zero-padding to the hour value in getCurrentTimeAndPeriod(similar to what we did with minutes).
It should be padded after converting to 12-hour format, otherwise, the modulo will reduce it to a single digit again.
hours = ${hours % 12 || 12}.padStart(2, '0');
Which should work well:

asirvadAbrahamVarghese · 2025-09-01T06:19:47Z

app/javascript/components/date-time-picker/index.jsx

+  };
+
+  const handleDateChange = (newDate) => {
+    if (newDate.length > 0) {


if (newDate.length) should be fine here since 0 is also falsy in JS and any number greater than 0 will be truthy

asirvadAbrahamVarghese · 2025-09-01T06:20:37Z

app/javascript/components/date-time-picker/index.jsx

+    const newTime = event.target.value;
+    setTime(newTime);
+    validateTime(newTime);
+    if (isValid) onChange({ value: combinedDateTime(), initialData });


Tried "11:5" in the time field, which doesn't match our pattern, so it's treated as an invalid time, the regex correctly returns false, which we also assign to the isValid state:

Given the condition(if (isValid) onChange({ ... });), we don't expect the onChange prop to be triggered in this case but it still gets called since isValid is still true(initial state value is true):

This is because state updates are asynchronous & batched and won't be reflecting immediately, when setIsValid is called react schedules the state update and updated value will be available only in the next render(only after the current event scope) not immediately after the call. So within the same event, we will still be accessing the previous state(which is true).

Returning the regex result from validateTime and using it for both setting state and controlling onChange should work fine:

const validateTime = (value) => { const timeRegex = /^(0[1-9]|1[0-2]):[0-5][0-9]$/; // Matches 12-hour format hh:mm return timeRegex.test(value); }; const handleTimeChange = (event) => { const newTime = event.target.value; setTime(newTime); const isValidTime = validateTime(newTime); setIsValid(isValidTime); if (isValidTime) onChange({ value: combinedDateTime(), initialData }); };

asirvadAbrahamVarghese · 2025-09-01T06:20:58Z

app/javascript/components/date-time-picker/index.jsx

+    const newTime = event.target.value;
+    setTime(newTime);
+    validateTime(newTime);
+    if (isValid) onChange({ value: combinedDateTime(), initialData });


The combinedDateTime function is also impacted the same way, we are updating the time state from here & then trying to access the state value from the same event scope, we will be having the previous time value due to react's batched updates.
Time field value is updated to "12:53" but time state will still provide its previous value "12:5" inside the same method scope:

Instead of relying on stale state we could update combinedDateTime to use the same input we're using for the state (in a way that it uses parameter value if available or the state value, thus other state values(date & period) are set):

const combinedDateTime = ({ dateValue, timeValue, periodValue } = {}) => `${dateValue || date} ${timeValue || time} ${periodValue || period}`;

🔝 we can also avoid the intermediate variable(const dateTime) by returning the string immediately

Now from handleTimeChange we can pass the value for time like:

if (isValidTime) { onChange({ value: combinedDateTime({ timeValue: newTime }), initialData, }); }

So that correct updated time is returned from combinedDateTime

asirvadAbrahamVarghese · 2025-09-01T06:21:21Z

app/javascript/components/date-time-picker/index.jsx

+        year: 'numeric',
+      }).format(newDate[0]);
+      setDate(formattedDate);
+      onChange({ value: combinedDateTime(), initialData });


Like discussed below, due to batched state updates we might want to update combinedDateTime to receive the updated date string:

const combinedDateTime = ({ dateValue, timeValue, periodValue } = {}) => `${dateValue || date} ${timeValue || time} ${periodValue || period}`;

and then from handleDateChange pass the date string value:

const handleDateChange = (newDate) => { if (newDate.length) { const formattedDate = new Intl.DateTimeFormat('en-US', { ..... onChange({ value: combinedDateTime({ dateValue: formattedDate }), initialData, }); } };

asirvadAbrahamVarghese · 2025-09-01T06:21:42Z

app/javascript/components/date-time-picker/index.jsx

+
+  const handlePeriodChange = (event) => {
+    setPeriod(event.target.value);
+    onChange({ value: combinedDateTime(), initialData });


Same as above, due to batched state updates we might want to update combinedDateTime to receive the updated period string:

const combinedDateTime = ({ dateValue, timeValue, periodValue } = {}) => `${dateValue || date} ${timeValue || time} ${periodValue || period}`;

and then from handlePeriodChange pass the updated period value:

const handlePeriodChange = (event) => { const newPeriod = event.target.value; setPeriod(newPeriod); onChange({ value: combinedDateTime({ periodValue: newPeriod }), initialData, }); };

asirvadAbrahamVarghese · 2025-09-01T06:22:00Z

app/javascript/components/date-time-picker/index.jsx

+
+  return (
+    <div>
+      <FormLabel>{field.label}</FormLabel>


Is the label prop expected to always be provided? The label element still gets rendered in the DOM even when no label is provided:

conditionally rendering it can help prevent unused elements in the DOM:
{label && <FormLabel>{label}</FormLabel>}
So that Label won't be rendered:

In this case, {field.label} will always be present. It comes from the dynamic-field-configuration file

defaultDateTimePickerValue: { **label: __('Default value')**, name: 'value', field: 'date-time-picker' },

asirvadAbrahamVarghese · 2025-09-01T06:26:03Z

app/javascript/components/date-time-picker/index.jsx

+  SelectItem,
+  FormLabel,
+} from 'carbon-components-react';
+import { getCurrentDate, getCurrentTimeAndPeriod } from '../service-dialog-form/helper';


Dealing with dates can be bug-prone. we should be fine for now with simple date logics but as logic grows, pulling in a utility like date-fns would help.

asirvadAbrahamVarghese · 2025-10-23T08:52:57Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-radio-button.jsx

+        updateFieldProps={handleFieldUpdate}
+        dynamicFieldAction={(event, inputProps) => fieldActions(event, inputProps)}
+        fieldConfiguration={radioButtonEditFields()}
+        dynamicToggleAction={(isDynamic) => resetEditModalTabs(isDynamic)}


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:54:05Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-tag-control.jsx

+
+/** Component to render a Field. */
+const DynamicTagControl = ({ dynamicFieldData: { section, field, fieldPosition }, onFieldAction }) => {
+  const { tabId, sectionId } = section;


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:54:11Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-tag-control.jsx

+  const inputId = `tab-${tabId}-section-${sectionId}-field-${fieldPosition}-tag-control`;
+  const editActionType = SD_ACTIONS.field.edit;
+
+  const refreshEnabledFields = section.fields


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:54:14Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-tag-control.jsx

+        }));
+      });
+    }
+  }, [fieldState.categories.length]);


I hope we’re setting the categories only on the initial render. If so, we can pass an empty array to useEffect instead of length. Is ESLint complaining about that?

asirvadAbrahamVarghese · 2025-10-23T08:54:19Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-tag-control.jsx

+    }));
+  };
+
+  const ordinaryTagControlOptions = () => ([


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:54:23Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-tag-control.jsx

+    return tabs;
+  };
+
+  const sortedItems = () => {


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-radio-button.jsx

asirvadAbrahamVarghese · 2025-10-23T08:54:31Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-tag-control.jsx

+        dynamicFieldAction={(event, inputProps) => fieldActions(event, inputProps)}
+        fieldConfiguration={tagControlEditFields()}
+        dynamicToggleAction={(isDynamic) => resetEditModalTabs(isDynamic)}
+        setCategoryData={(cat, subCat) => setCategoryData(cat, subCat)}


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:55:08Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-text-area.jsx

+
+/** Component to render a Field. */
+const DynamicTextArea = ({ dynamicFieldData: { section, field, fieldPosition }, onFieldAction }) => {
+  const { tabId, sectionId } = section;


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:55:12Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-text-area.jsx

+
+  const inputId = `tab-${tabId}-section-${sectionId}-field-${fieldPosition}-text-area`;
+  const editActionType = SD_ACTIONS.field.edit;
+  const refreshEnabledFields = section.fields


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:55:16Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-text-area.jsx

+    setFieldState((prevState) => ({ ...prevState, dynamic: isDynamic }));
+  };
+
+  const ordinaryTextAreaOptions = () => ([


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:55:33Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-text-area.jsx

+        updateFieldProps={handleFieldUpdate}
+        dynamicFieldAction={(event, inputProps) => fieldActions(event, inputProps)}
+        fieldConfiguration={textAreaEditFields()}
+        dynamicToggleAction={(isDynamic) => resetEditModalTabs(isDynamic)}


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:56:15Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-text-input.jsx

+
+/** Component to render a Field. */
+const DynamicTextInput = ({ dynamicFieldData: { section, field, fieldPosition }, onFieldAction }) => {
+  const { tabId, sectionId } = section;


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:56:18Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-text-input.jsx

+  const inputId = `tab-${tabId}-section-${sectionId}-field-${fieldPosition}-text-input`;
+  const editActionType = SD_ACTIONS.field.edit;
+
+  const refreshEnabledFields = section.fields


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:56:24Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-text-input.jsx

+    setFieldState((prevState) => ({ ...prevState, dynamic: isDynamic }));
+  };
+
+  const ordinaryTextBoxOptions = () => ([


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:56:25Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-text-input.jsx

+          placeholder={__('Default value')}
+          value={fieldState.value}
+          readOnly={fieldState.readOnly}
+          onChange={(e) => handleFieldUpdate(e, { value: e.target.value })}


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:56:28Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-text-input.jsx

+        updateFieldProps={handleFieldUpdate}
+        dynamicFieldAction={(event, inputProps) => fieldActions(event, inputProps)}
+        fieldConfiguration={textBoxEditFields()}
+        dynamicToggleAction={(isDynamic) => resetEditModalTabs(isDynamic)}


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:57:04Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-time-picker.jsx

+        dynamicFieldAction={(event, inputProps) => fieldActions(event, inputProps)}
+        fieldConfiguration={timePickerEditFields()}
+        dynamicToggleAction={(isDynamic) => resetEditModalTabs(isDynamic)}
+        onValueChange={(value) => updateDateTime(value)}


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:58:15Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-time-picker.jsx

+
+/** Component to render a Field. */
+const DynamicTimePicker = ({ dynamicFieldData: { section, field, fieldPosition }, onFieldAction }) => {
+  const { tabId, sectionId } = section;


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:58:17Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-time-picker.jsx

+    position: fieldPosition,
+    name: fieldValues.name || inputId,
+    label: fieldValues.label || __('Timepicker'),
+    fieldsToRefresh: section.fields


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:58:21Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-time-picker.jsx

+    setFieldState((prevState) => ({ ...prevState, value }));
+  };
+
+  const ordinaryTimePickerOptions = () => ([


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-checkbox.jsx

asirvadAbrahamVarghese · 2025-10-23T08:58:23Z

app/javascript/components/service-dialog-form/dynamic-fields/dynamic-time-picker.jsx

+        <DatePicker
+          datePickerType="single"
+          onChange={handleDateChange}
+          minDate={fieldState.showPastDates ? undefined : new Date().toLocaleDateString()}


Same thoughts as I shared(here) on app/javascript/components/service-dialog-form/dynamic-fields/dynamic-date-picker.jsx

GilbertCherrie · 2025-10-27T18:58:51Z

@elsamaryv This is a good start so far but lots of issues that still need to be fixed. We can tackle them a bit at a time to make it easier. To start here are the issues I found so far:

Overall

Not needed but if possible try adding a Reset Button to the Edit form
Form submit error handling, show error
Modal keeps closing when you hover away from the page
Deleting a tab should move the focus to the tab on the left, not the right. This avoids the focus moving to the Create Tab tab which is empty
Saving a default value for 1 field deletes this value for other fields, not sure why
DO NOT FIX THIS ONE YETL: we can leave this for the final review. This can be avoided possibly and use random number, need to ask Jason first if random number is ok. Need to try to make Default Field name to be formatted like text_box_1, text_box_2. Create array of all field names for each field type and try to create text_box_1. If text_box_1 exists then name is text_box_2. If text_box_2 exists try text_box_3 and repeat until a valid name is found.
Common modal issues:
- Help field doesn’t save
- Validation fields don’t save
- Validator field missing tooltip saying Regular Expression

Tabs

Try to add carbon icon to the Create Tab label to make it look like the old form
Issue with saving the tab description

Textbox

Value type field doesn’t save

Text Area

Dialog Editor form can’t save when this field is required and is empty

asirvadAbrahamVarghese · 2025-10-28T06:22:24Z

app/javascript/components/service-dialog-form/dynamic-section.jsx

+const DynamicSection = ({ section, sectionAction }) => {
+  const [sectionData, setSectionData] = useState({ maximize: true });
+
+  const { sectionId, tabId, title } = section;


Maybe move the props destructuring to the top?

const DynamicSection = ({ section, sectionAction }) => { const { sectionId, tabId, title } = section; const [sectionData, setSectionData] = useState({ maximize: true }); ...

asirvadAbrahamVarghese · 2025-10-28T06:22:29Z

app/javascript/components/service-dialog-form/dynamic-section.jsx

+
+  const renderMinMaxButton = () => (
+    <Button
+      renderIcon={sectionData.maximize ? Minimize16 : Maximize16}


I don’t really know what Minimize16 and Maximize16 icons are, but reading this, shouldn’t it be sectionData.maximize ? Maximize16 : Minimize16 instead of sectionData.maximize ? Minimize16 : Maximize16 ?

asirvadAbrahamVarghese · 2025-10-28T06:22:38Z

app/javascript/components/service-dialog-form/dynamic-section.jsx

+        ))
+      }
+      {
+        (!section.fields || section.fields.length === 0) && renderHelpText()


We should be able to do !section?.fields?.length && renderHelpText() once we upgrade Babel, Webpack, etc.

asirvadAbrahamVarghese · 2025-10-28T06:22:58Z

app/javascript/components/service-dialog-form/dynamic-section.jsx

+    <div
+      className="dynamic-section"
+      draggable
+      onDragStart={(event) => onSectionAction(SD_ACTIONS.onDragStartSection, event)}


I saw that when the edit field modal is open over our draggable elements, I can drag the whole UI like this:

This might be because we’re not preventing the browser’s default behavior in the onDragStart event:

const handleDragStart = (event) => { event.preventDefault(); onSectionAction(SD_ACTIONS.onDragStartSection, event); };

and then:

return ( <div className="dynamic-section" draggable onDragStart={handleDragStart}

asirvadAbrahamVarghese · 2025-10-28T06:23:20Z

app/javascript/components/service-dialog-form/dynamic-section.jsx

+      className="dynamic-section"
+      draggable
+      onDragStart={(event) => onSectionAction(SD_ACTIONS.onDragStartSection, event)}
+      onDragEnter={(event) => onSectionAction(SD_ACTIONS.onDragEnterSection, event)}


Just a thought, do we actually need both onDragEnter and onDragOver events?

asirvadAbrahamVarghese · 2025-10-28T06:23:30Z

app/javascript/components/service-dialog-form/dynamic-section.jsx

+      onDragOver={(event) => onSectionAction(SD_ACTIONS.onDragOverListener, event)}
+      tab={tabId}
+      section={sectionId}
+      id={`dynamic-tab-${tabId}-section-${sectionId.toString()}`}


Maybe pull this into a const and reuse it for both id & key. Also, we don’t need toString() here since it’s already being concatenated inside a template literal(``):

const idString = `dynamic-tab-${tabId}-section-${sectionId}`

asirvadAbrahamVarghese · 2025-10-28T06:23:44Z

app/javascript/components/service-dialog-form/dynamic-section.jsx

+      tab={tabId}
+      section={sectionId}
+      id={`dynamic-tab-${tabId}-section-${sectionId.toString()}`}
+      key={`dynamic-tab-${tabId}-section-${sectionId.toString()}`}


I just noticed there’s an issue with how sectionId is being used in the key here
(just pointing to the id value here, since it’s the same one we use for both id and key)
Steps to reproduce:

Setting up 2 sections with some fields here:

2) Removing the first one: 3) Now adding another section and looking into the id value we can see that both are same 4) Now try to add a field to the second section 5) But that ends up on to the first section:
Just like we discussed in this comment about react keys, this was also because react got confused since two elements have the same key and incorrectly updated the wrong component.
I am guessing, problem could be here where we are using sections.length as sectionId

miq-bot added enhancement wip labels Aug 25, 2025

Fryguy reviewed Aug 25, 2025

View reviewed changes

app/javascript/packs/component-definitions-common.js Outdated Show resolved Hide resolved

Fryguy reviewed Aug 25, 2025

View reviewed changes

app/javascript/packs/component-definitions-common.js Outdated Show resolved Hide resolved

Fryguy assigned GilbertCherrie and asirvadAbrahamVarghese Aug 25, 2025

elsamaryv marked this pull request as ready for review August 26, 2025 10:55

elsamaryv requested a review from a team as a code owner August 26, 2025 10:55

asirvadAbrahamVarghese reviewed Aug 27, 2025

View reviewed changes

app/javascript/components/common/inline-flash-message/index.jsx Show resolved Hide resolved

asirvadAbrahamVarghese reviewed Aug 27, 2025

View reviewed changes

asirvadAbrahamVarghese reviewed Sep 1, 2025

View reviewed changes

elsamaryv changed the title ~~[WIP] Conversion of Service Dialogs Form from Angular to React~~ Conversion of Service Dialogs Form from Angular to React Sep 11, 2025

elsamaryv force-pushed the service-dialogs-conversion branch from 78be4a5 to ad336c6 Compare October 7, 2025 09:36

asirvadAbrahamVarghese reviewed Oct 23, 2025

View reviewed changes

asirvadAbrahamVarghese reviewed Oct 28, 2025

View reviewed changes

GilbertCherrie mentioned this pull request Nov 5, 2025

Include invalid workflows and disable them in the embedded workflow component #9708

Open

		@@ -0,0 +1,37 @@
		/* eslint-disable no-undef */

		// Look for notification popups and disable them if present

Conversion of Service Dialogs Form from Angular to React #9592

Are you sure you want to change the base?

Conversion of Service Dialogs Form from Angular to React #9592

Uh oh!

Conversation

elsamaryv commented Aug 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fryguy commented Aug 25, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elsamaryv Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asirvadAbrahamVarghese Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asirvadAbrahamVarghese Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asirvadAbrahamVarghese Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elsamaryv Aug 28, 2025 •

edited

Loading

asirvadAbrahamVarghese Aug 27, 2025 •

edited

Loading

asirvadAbrahamVarghese Aug 27, 2025 •

edited

Loading

asirvadAbrahamVarghese Aug 27, 2025 •

edited

Loading